Bump server to 1.26.2, enable HTTP by default#734
Bump server to 1.26.2, enable HTTP by default#734bergundy merged 5 commits intotemporalio:next-serverfrom
Conversation
bergundy
force-pushed
the
next-server
branch
from
298569b to
7c407f3
Compare
temporalcli/commandsgen/commands.yml
Outdated
| Port for the HTTP API service. | ||
| Default is off. | ||
| description: Port for the HTTP API service. | ||
| default: 7243 |
There was a problem hiding this comment.
Can we get confirmation that enabling HTTP by default is only for an internal Nexus completion callback mechanism? If so, can we instead use an in-memory approach instead of binding this port for everyone by default? (some internal team discussions are underway on this)
There was a problem hiding this comment.
It's used for callbacks and for all other Nexus RPCs in the spec.
Available to be called from external clients or internally by the history service Nexus machinery.
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
Mostly LGTM (just a minor comment or two before approval), but would like @josh-berry to re-confirm his approval confirming he is ok with http-always-on-default-random-port.
| description: | | ||
| Port for the HTTP API service. | ||
| Default is off. | ||
| description: Port for the HTTP API service. Defaults to a random free port. |
There was a problem hiding this comment.
Should we have any way to disable the HTTP API service? I can't think of a good reason at this time, so can probably not worry about it for now.
There was a problem hiding this comment.
I thought about this but don't think it's a concern if it defaults to a random port. We can always add a --no-http flag if we wanted later.
|
|
||
| func TestServer_StartDev_Simple(t *testing.T) { | ||
| port := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) | ||
| httpPort := strconv.Itoa(devserver.MustGetFreePort("127.0.0.1")) |
There was a problem hiding this comment.
Why are these test updates needed? Can't we just let it choose the random default?
There was a problem hiding this comment.
They're not, I left it here in prep for when we make the HTTP port fixed.
There was a problem hiding this comment.
I don't think that's a reasonable assumption/expectation yet. But so long as there's some code path somewhere that is covering the random part, all good, even if it's not needed.
temporalcli/commands.server.go
Outdated
|
|
||
| cctx.Printer.Printlnf("CLI %v\n", VersionString()) | ||
| cctx.Printer.Printlnf("%-8s %v:%v", "Server:", toFriendlyIp(opts.FrontendIP), opts.FrontendPort) | ||
| cctx.Printer.Printlnf("%-8s %v:%v", "HTTP:", toFriendlyIp(opts.FrontendIP), opts.FrontendHTTPPort) |
There was a problem hiding this comment.
I am a little concerned about this part. The HTTP API is not considered stable and is not really documented. So listing this as though it is I think is a bit rough. Can we only display this when the port was given explicitly?
There was a problem hiding this comment.
Hmm... for metrics we print even if the port isn't specified. Let's keep this consistent.
There was a problem hiding this comment.
The concern is not about whether the port is specified, it's about promoting the HTTP API. Metrics is stable/GA and documented w/ a cloud equivalent, HTTP API is not. We should not by default encourage use of the HTTP API in dev server at this time until it is a documented product available to all cloud users by default. But if a user explicitly sets an option, that's different/understandable, though I would also be ok just not showing it at all.
If it's callable externally, then I think my preference would be to make it non-random by default. Otherwise we are making it harder for Nexus users to know what port to use. This is a weak preference since we can always make it non-random later. I have no objection to merging as-is if there are strong opinions in the other direction. |
I agree with everything you said. We're going to have to make it non-random eventually, but let's prepare the SDKs for that first or figure out a way to reuse the gRPC port for Nexus. |
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM (assuming per #734 (comment) that there is some test that covers the random HTTP port aspect)
|
@bergundy Do you think you'll be able to merge this today? |
3 participants
No description provided.